Conversation
|
r? @chenyukang rustbot has assigned @chenyukang. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
I'll add tests for assembly output and for target incompatibility. |
This comment has been minimized.
This comment has been minimized.
|
|
||
| fn packed_stack_attr<'ll>(cx: &SimpleCx<'ll>, sess: &Session) -> Option<&'ll Attribute> { | ||
| if sess.target.arch != Arch::S390x { | ||
| return None; |
There was a problem hiding this comment.
Maybe panic here given that the frontend should already have emitted an error.
There was a problem hiding this comment.
hmm packed_stack_attr(...) is called every time llfn_attrs_from_instance is called indifferent of target arch.
And I think it would not be a good thing to add an arch check to llfn_attrs_from_instance as there are no other checks in that function. It blindly calls all possibilities and assumes that checks are already handled elsewhere.
5ddcabb to
4ebec9d
Compare
| if sess.opts.unstable_opts.packed_stack { | ||
| if sess.target.arch != Arch::S390x { | ||
| sess.dcx().emit_err(errors::UnsupportedPackedStack); | ||
| } |
There was a problem hiding this comment.
This seems like the most reasonable place to check the backchain condition?
There was a problem hiding this comment.
yes i will move the other check here
There was a problem hiding this comment.
unfortunately feature flags are only parsed during codegen_backend.init(sess) which is one step after rustc_session::build_session(...). So no feature flags available to check at this point.
-> i need to check them in codegen_llvm or codegen_ssa.
There was a problem hiding this comment.
Please avoid duplicating the check across the codegen backends.
The check seems to be somewhat similar in nature to
. So maybe renamecheck_abi_required_features to something slightly more general and do the check there?
There was a problem hiding this comment.
yes this seems, also to me, the only option left.
There was a problem hiding this comment.
there is another way.
I could try to talk to the llvm guys if its possible to make packed-stack a proper target-feature so we dont have to go over a cli argument.
There was a problem hiding this comment.
Not sure if that would help. Target feature conflicts currently also need ad-hoc checks, we don't have a framework for dealing with them at the moment.
| If not specified, overflow checks are enabled if | ||
| [debug-assertions](#debug-assertions) are enabled, disabled otherwise. | ||
|
|
||
| ## packed-stack |
There was a problem hiding this comment.
Since the option isn't stable yet, this is the wrong place to document it I think?
There was a problem hiding this comment.
If i dont have packed-stack here x test tidy will complain. I try to find another location to do it.
There was a problem hiding this comment.
Something must be going wrong there. The page is explicitly about -C flags, not -Z flags.
There was a problem hiding this comment.
you where right. The error was not about my changes but about some uncommitted local changes. :( sorry
everything works as expected.
This comment has been minimized.
This comment has been minimized.
this enables packed-stack just as -mpacked-stack in clang and gcc. packed-stack is needed on s390x for kernel development.
0d6d167 to
0c182af
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
this enables
-Zpacked-stackjust as-mpacked-stackin clang and gcc. packed-stack is needed on s390x for kernel development.For reference: #151154 and #150766
look at @uweigand s post for full explanation of what this does. Here a wrap-up:
#150766 (comment)